Reviewed with a team of six agents (RL/codebase, vLLM, Automodel, tests, bug-hunting, and triage of the existing threads), against vLLM 0.25.1 (752a3a50) and Automodel 1814c6c9. Every finding below was re-verified against source before posting.

Nice work — this is a large, genuinely difficult integration and it holds up well under scrutiny. Highlights from what we checked and found clean:

• nemo_rl/data/deepseek_v4_encoding.py is byte-identical to vLLM 0.25.1's copy apart from one added license line — no adaptation drift.
• All three non-obvious vLLM APIs are used correctly: replace_parameter(..., prefer_copy=True), deepgemm_post_process_fp8_weight_block(..., is_bmm=, bmm_batch_size=), and the get_layerwise_info(...).restore_metadata / SKIP_TENSORS lifecycle. The ordering around record_metadata_for_reloading is the only window that works, and the PR hits it.
• The TP1/CP8/EP128 recipe is valid and internally consistent (ep_shard_size = 1 is the correct branch at exact equality; EP128 divides n_routed_experts: 256).
• Every DeepSeek V4 test genuinely executes — 0 skipped, confirmed three ways (item-count reconciliation, the runner's nodeid manifest, and a negative control showing the marker filter bites). ruff format, ruff check, pyrefly, and config_cli.py minimize-check are all clean.
• The prior round's report of 21 failing committed tests does not reproduce at this HEAD; all 21 existing review threads are stale or fixed, and we've resolved them.

Two notes on scope, so the findings below read correctly. Several are latent rather than live: the rendering issue is not on this recipe's GRPO path, and the dp_size guard is pre-existing code your recipe is simply the first to make degenerate. They're raised because they'd bite the next person, not because they affect your validated run.

One request: would you be able to share throughput numbers for the run behind the reference curves — tokens/sec and peak memory at the 16-node TP1/CP8/EP128 configuration? The convergence curves through step 50 are great for correctness, but there's no baseline for what the FP8 refit path costs per step, which is what the next person sizing a DSV4 run will want.

Generated by Claude Code

Follow-up to my earlier review with three findings that landed after I published it -- my mistake for posting before the whole team had reported back. Two of these are things I should have caught in the first pass.

• data.filter_overlong_prompts: true is read by nothing. It appears exactly once in the repo (this recipe) and is not a DataConfig field. Same class as the dead NRL_* env vars already flagged.
• deepseek_v4_fp8.py is missing from pyrefly.toml while both its quantization siblings are listed, which is the only reason pyrefly check reports clean. Adding it surfaces one real error. This qualifies my earlier "lint is clean at HEAD" note: it is clean, but the PR's largest new module is not being typechecked.
• A stride-shaped hole in the DSV4 test suite, found by mutation testing rather than inspection. The production code is correct; the suite simply would not catch one specific silent regression. A verified test is included.

Everything else from the first review stands unchanged.

Generated by Claude Code

Final follow-up -- four test-quality findings, closing out the review. Sorry for the trailing reviews; the test agent's mutation work finished after I'd already posted, and these were worth not dropping.

• num_special_tokens_to_add returns 1 instead of 0 against a real HF tokenizer. This sharpens my earlier comment on the same area: the issue is not only that the test is weak, the production method is wrong and the stub masks it.
• process_weights_after_loading_moe has zero test coverage repo-wide, and this PR changed how it manages parameter storage. The sibling linear path has an incident-driven guard for exactly this failure mode; the MoE path holds more memory and has none. Verified test included.
• The narrowed grpo.setup() KV-cache guard is untested on either side of the change.
• A finalize_refit negative test cannot fail -- confirmed by mutation.

All findings verified against source; both included tests were run at HEAD and mutation-tested to confirm they are not tautologies.

To restate the overall picture, since this is the third review and the volume may read worse than the PR deserves: the implementation holds up well. The vendored encoder is byte-identical to upstream, all three non-obvious vLLM APIs are used correctly, the CP8/EP128 parallelism is valid, and every DeepSeek V4 test genuinely executes with zero skips (proven four ways). Roughly as many candidate findings were killed under scrutiny as were posted -- including two of my own.

Generated by Claude Code

LGTM. Thanks!